Reads treat values in dropped blob files as deleted - #15649

Findings

🟡 MEDIUM

M1. EmbeddedAwareBlobFetcher inherits storage_with_dropped_blob_files_ but base fetcher's value is not proxied — db/blob/blob_fetcher.h

• Issue: storage_with_dropped_blob_files_ is placed on the BlobFetcher base class. EmbeddedAwareBlobFetcher inherits from BlobFetcher and decorates a base fetcher. If GetContext::blob_fetcher_ ever pointed to an EmbeddedAwareBlobFetcher, storage_with_dropped_blob_files() would return nullptr (the default) instead of the underlying VersionBlobFetcherBase's value, silently disabling the dropped blob check.

  In the current code, GetContext::blob_fetcher_ always points to a VersionBlobFetcherBase subclass, so this works. But the design is fragile.

• Suggested fix: Move storage_with_dropped_blob_files() and the protected member to VersionBlobFetcherBase, or add a comment on the base class documenting why it lives there (e.g., to avoid dynamic_cast in GetContext).

M2. remaining_columns lifetime in GetContext::SaveValue is fragile — table/get_context.cc

• Issue: remaining_columns is a stack-local std::string. After CheckDroppedBlobReferences returns kRemainingColumns, unpacked_value is reassigned to point into it. The Slice is consumed within the same function scope, so this is correct today. But the lifetime dependency between remaining_columns and unpacked_value is implicit and easy to break in future refactoring.

• Suggested fix: Consider adding a brief comment noting that unpacked_value may alias remaining_columns and both must stay alive through the switch.

M3. Double PrepareValueInternal() on the dropped-blob-check path in FindNextUserEntryInternal — db/db_iter.cc

• Issue: The diff adds PrepareValueInternal() for blob/entity entries before the switch. The existing switch case for kTypeBlobIndex/kTypeWideColumnEntity also calls PrepareValueInternal(). When the type is NOT converted to deletion (blob file is not dropped), the value is prepared twice. PrepareValueInternal is a no-op when already prepared, so this is safe but adds one extra function call + branch on the (rare) path where dropped blob files exist but the specific entry's blob file is NOT dropped.

• Suggested fix: Acceptable as-is. The UNLIKELY(!dropped_blob_files_.empty()) guard ensures this only runs when dropped files exist. No change needed.

🟢 LOW / NIT

L1. Feature-off cost: 24 bytes per DBIter + 8 bytes per BlobFetcher — db/db_iter.h, db/blob/blob_fetcher.h

• Issue: dropped_blob_files_ adds 24 bytes to every DBIter. storage_with_dropped_blob_files_ adds 8 bytes to every BlobFetcher. In the common case (no dropped files), these are zeros. Per the "feature-off cost is zero" principle, this is a small constant cost. The DBIter copy is needed for tailing iterator safety; the BlobFetcher pointer avoids repeated checks.

• Suggested fix: The trade-off is reasonable and well-motivated. Consider a brief comment on dropped_blob_files_ explaining why a copy (not a pointer) is needed.

L2. appendToReplayLog captures original type before conversion — table/get_context.cc

• Issue: The replay log at line 430 captures the original parsed_key.type and value before the dropped blob conversion. For transaction conflict detection this is correct (the log records what exists in storage). Worth noting for future readers.

• Suggested fix: None needed.

L3. New ticker not in Java — include/rocksdb/statistics.h

• Issue: PR description notes "C++ only because the Java ticker byte space is full." This is a known limitation.

• Suggested fix: Already documented.

L4. InstallCompactionResults condition change — db/compaction/compaction_job.cc:2765

• Issue: The old condition was compaction->skip_missing_blob_files() && ... && !storage_info->GetBlobFileMetaData(blob_file_number). The new condition is (compaction->skip_missing_blob_files() || storage_info->IsDroppedBlobFile(blob_file_number)). The old code additionally checked that the blob file is NOT in the Version's metadata. For skip_missing_blob_files, this extra check is now dropped. However, skip_missing_blob_files is set when best-efforts recovery drops blob files entirely, so they won't be in the Version either. For the dropped-blob-file case, IsDroppedBlobFile is the right check since dropped files are in the Version's metadata but marked as dropped. The logic is correct.

• Suggested fix: None needed.

Cross-Component Analysis

Positive Observations

• The design reuses IsDroppedBlobIndex across reads and compaction, ensuring reads return what compaction will produce.
• RemoveBlobColumns consolidates duplicated entity column filtering into a single reusable function with a clean callback interface.
• Copying dropped_blob_files_ for tailing iterators cleanly avoids Version lifetime issues.
• Comprehensive test coverage: 10 new test cases covering Get, MultiGet, iterators (forward, reverse, tailing, unprepared), merges, wide-column entities, timestamps, auto-compaction, GC, read-only, and blob direct write.
• UNLIKELY guards minimize feature-off cost on hot paths.
• RepairsMissingBlobReferences() efficiently checks only whether any input SST's oldest blob file could be in the dropped set, using the sorted property.

Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase